Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ODF actor implemented with Ray Tracing #869

Open
wants to merge 144 commits into
base: master
Choose a base branch
from

Conversation

tvcastillod
Copy link
Contributor

@tvcastillod tvcastillod commented Mar 8, 2024

This PR aims to create a new odf actor defined with raymarching based on the paper Ray Tracing Spherical Harmonics Glyphs . Work is still in progress.

A visual comparison with the current implementation of fury using polygons
image

An example is provided showing that it supports displaying multiple glyphs, and with different SH degree.
image

Copy link
Contributor

@guaje guaje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @tvcastillod, here are some minor comments on some of these files.

fury/actors/odf.py Outdated Show resolved Hide resolved
fury/actors/odf.py Outdated Show resolved Hide resolved
Copy link
Contributor

@guaje guaje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @tvcastillod!

Overall great work putting up together such a needed actor.

In addition to the changes outlined below, I suggest moving the content of rt_odfs to ray_tracing/odf.

fury/texture/tests/test_utils.py Show resolved Hide resolved
fury/utils.py Outdated Show resolved Hide resolved
fury/shaders/utils/find_roots.frag Outdated Show resolved Hide resolved
fury/shaders/utils/find_roots.frag Outdated Show resolved Hide resolved
fury/shaders/utils/find_roots.frag Outdated Show resolved Hide resolved
fury/shaders/rt_odfs/tournier/eval_sh_12.frag Outdated Show resolved Hide resolved
fury/shaders/rt_odfs/tournier/eval_sh_10.frag Outdated Show resolved Hide resolved
fury/shaders/rt_odfs/tonemap.frag Outdated Show resolved Hide resolved
fury/actors/odf.py Outdated Show resolved Hide resolved
fury/shaders/rt_odfs/eval_sh.frag Outdated Show resolved Hide resolved
@tvcastillod tvcastillod marked this pull request as ready for review September 20, 2024 16:27
@guaje guaje requested a review from skoudoro September 23, 2024 22:28
Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exciting, thank you for this work @tvcastillod !

First, can you fix the issues from code format CI's and build doc CI's

then, see my first initial comments below.

Question: Can you explain a bit how did you get all this eval_sh_*?

@@ -4019,3 +4020,73 @@ def uncertainty_cone(
angles = main_dir_uncertainty(evals, evecs, signal, sigma, b_matrix)

return double_cone(centers, evecs, angles, colors, scales, opacity)


@warn_on_args_to_kwargs()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed since this is a new function

Type of basis (descoteaux, tournier)
'descoteaux' for the default ``descoteaux07`` DYPY basis.
'tournier' for the default ``tournier07` DYPY basis.
scales : float or ndarray (N, )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

, optional to add

'tournier' for the default ``tournier07` DYPY basis.
scales : float or ndarray (N, )
ODFs size.
opacity : float
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

, optional to add

----------
data: ndarray
2D array
axis: int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

, optional to add

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants